feat(agent): gather thread_ts or message ts of events outside of assistant context#1444
feat(agent): gather thread_ts or message ts of events outside of assistant context#1444zimeg wants to merge 5 commits intozimeg-feat-agent-set-suggested-promptsfrom
Conversation
zimeg
added
enhancement
zimeg
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## zimeg-feat-agent-set-suggested-prompts #1444 +/- ##
=======================================================================
Coverage 90.66% 90.67%
=======================================================================
Files 226 226
Lines 7202 7205 +3
=======================================================================
+ Hits 6530 6533 +3
Misses 672 672 ☔ View full report in Codecov by Sentry. |
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
🗣️ A few comments of thoughts for reviewers so kind!
|
|
||
| # Resolve thread_ts: assistant events set context.thread_ts, otherwise read from event | ||
| event = request.body.get("event", {}) | ||
| thread_ts = request.context.thread_ts or event.get("thread_ts") or event.get("ts") |
There was a problem hiding this comment.
📝 note: We avoided adding a new ts value to the listener context in this PR and instead use the event information. This was to keep scope changes minimal and parity with the Bolt JS implementation IIRC.
| @@ -218,6 +218,9 @@ def extract_thread_ts(payload: Dict[str, Any]) -> Optional[str]: | |||
| # This utility initially supports only the use cases for AI assistants, but it may be fine to add more patterns. | |||
| # That said, note that thread_ts is always required for assistant threads, but it's not for channels. | |||
| # Thus, blindly setting this thread_ts to say utility can break existing apps' behaviors. | |||
There was a problem hiding this comment.
👁️🗨️ thought: I'm surprised that say posts top-level messages in response to threaded messages by default TBH!
I agree that a "fix" for this, to respond in thread if a thread_ts is present, might cause new behavior for apps but am wondering if this is intended behavior or something to ponder changing in the future?
There was a problem hiding this comment.
hmmm good question! id love to hear what changing it in the future would look like 🤔 to me it makes sense for apps to respond in thread
| # Resolve thread_ts: assistant events set context.thread_ts, otherwise read from event | ||
| event = request.body.get("event", {}) | ||
| thread_ts = request.context.thread_ts or event.get("thread_ts") or event.get("ts") | ||
|
|
There was a problem hiding this comment.
nice ⭐ thanks for fixing this!
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
question: @zimeg Thanks a bunch for this PR! I'll look over it today.
I noticed that thread_ts is set to either thread_ts or ts. Is this the behaviour we want throughout all of Bolt? While chat_stream requires a thread_ts argument that must be ts when thread_ts isn't present... I don't think that's consistent with other methods. My concern is that setting thread_ts to a non-thread message could have unexpected side effects.
3 participants
Summary
This PR gathers the
thread_tsor messagetsof events and saves these as the internal thread for theagentargument 👾Fixes an issue where streamed responses and set status errors in a public channel.
Testing
slack-samples/bolt-python-assistant-template#51
Category
slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsRequirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.